Skip to content

Conversation

@hexfusion
Copy link
Contributor

This PR is a cherry-pick of a1f5d26 reverted by #21.

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2019
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 18, 2019
@hexfusion
Copy link
Contributor Author

/test e2e-aws

@hexfusion
Copy link
Contributor Author

we are going to give this another go PTAL

/cc @smarterclayton

@hexfusion
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2019
@hexfusion
Copy link
Contributor Author

/test e2e-aws

@smarterclayton
Copy link

@sttts requested we move this to a more appropriate place. Where is this value actually used? Machine-config-operator? If so it belongs there.

@hexfusion
Copy link
Contributor Author

I am sure you know this but it is unclear to me where that makes it land.

@smarterclayton
Copy link

smarterclayton commented Mar 18, 2019

@abhinavdahiya if it's consumed by something in MCO I think the best place to take the reference is there. This code knows nothing about specific components, and even though in the future I could see us moving bootkube.sh to functionality in this repo at the current time it's a thin shim deliberately. Opinions?

@abhinavdahiya
Copy link
Contributor

@abhinavdahiya if it's consumed by something in MCO I think the best place to take the reference is there. This code knows nothing about specific components, and even though in the future I could see us moving bootkube.sh to functionality in this repo at the current time it's a thin shim deliberately. Opinions?

The MCO consumes the kube-client-agent to request etcd certs from kubernetes CSR api and therefore we have that declared as dependency in MCO repo https://github.com/openshift/machine-config-operator/blob/master/install/image-references#L41

the kube-etcd-signer-server is used to bootstrap the etcd cluster by mimic-ing the kubernetes CSR api on the bootstrap node. So this is image is more to the effect of special component used on the bootstrap node required for cluster bootstrapping and therefore, IMO this is correct location for this and not dirty hack as per here

@smarterclayton
Copy link

Is bootkube the component that creates the reference?

@abhinavdahiya
Copy link
Contributor

Is bootkube the component that creates the reference?

Hmm 😕 what reference ?

@smarterclayton
Copy link

The reference to the image kube-etcd-signer-server by calling cluster-version-operator image?

@abhinavdahiya
Copy link
Contributor

The reference to the image kube-etcd-signer-server by calling cluster-version-operator image?

from @hexfusion comment

@smarterclayton the image is used by bootstrap node via installer https://github.com/openshift/installer/blob/af56e2db2e2601156a1ebe56269c7deb67f3c725/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L225.

the bootkube.sh service on the bootstrap node calls the reference.

@smarterclayton
Copy link

Why doesn't installer take the reference?

@abhinavdahiya
Copy link
Contributor

Why doesn't installer take the reference?

cluster-bootstrap is also only used by installer on bootstrap node and we have image decalaration in this repo

- name: cluster-bootstrap
from:
kind: DockerImage
name: docker.io/openshift/origin-cluster-bootstrap:v4.0
so with current setup kube-etcd-signer-server doesn't feel any different.

IMO keeping the image requirements for bootstrap node here cluster-bootstrap keeps it more compartmentalized.. ?

@hexfusion
Copy link
Contributor Author

@smarterclayton @abhinavdahiya have we come to an agreement?:) This is now a blocker for etcd metrics.

@hexfusion
Copy link
Contributor Author

/retest

@hexfusion
Copy link
Contributor Author

@sttts would you mind chiming in on this I know you were against this initially.

@sttts
Copy link
Contributor

sttts commented Mar 27, 2019

@hexfusion dependencies for bootkube.sh do belong where bootkube.sh is stored. Today, cluster-bootstrap knows nothing about the bootkube.sh bootstrapping process. Any etcd dependency here is on the wrong layer: cluster-bootstrap is only a generic tool launching some static pods and creating some manifests. It has no knowledge about what it actually creates.

I could imagine a world where we move the bootkube.sh logic into cluster-bootstrap, like a cluster-bootstrap render bootkube.sh | bash command called by the bootstrap node. If all are on the same page with this vision, this PR would be in line.

@hexfusion
Copy link
Contributor Author

/test e2e-aws

@abhinavdahiya
Copy link
Contributor

@hexfusion dependencies for bootkube.sh do belong where bootkube.sh is stored. Today, cluster-bootstrap knows nothing about the bootkube.sh bootstrapping process. Any etcd dependency here is on the wrong layer: cluster-bootstrap is only a generic tool launching some static pods and creating some manifests. It has no knowledge about what it actually creates.

I could imagine a world where we move the bootkube.sh logic into cluster-bootstrap, like a cluster-bootstrap render bootkube.sh | bash command called by the bootstrap node. If all are on the same page with this vision, this PR would be in line.

cluster-bootstrap render bootkube.sh | bash makes sense

@mfojtik
Copy link
Contributor

mfojtik commented Mar 27, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hexfusion, mfojtik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2019
@hexfusion
Copy link
Contributor Author

/test e2e-aws

4 similar comments
@hexfusion
Copy link
Contributor Author

/test e2e-aws

@hexfusion
Copy link
Contributor Author

/test e2e-aws

@hexfusion
Copy link
Contributor Author

/test e2e-aws

@hexfusion
Copy link
Contributor Author

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@hexfusion
Copy link
Contributor Author

/test e2e-aws

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants